-
Notifications
You must be signed in to change notification settings - Fork 3
limit offered by facet to specific offerors #2692
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
OpenAPI ChangesShow/hide 223 changes: 0 error, 0 warning, 223 info |
| aggregations: { | ||
| ...data.metadata.aggregations, | ||
| offered_by: data.metadata.aggregations.offered_by.filter( | ||
| (value) => value && value.key in offerors, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will leave review to @abeglova , but skimmed this and wanted to mention:
It's best to avoid in operator in JS, unfortunately. It dates from the dark(er) ages of JS (AI told me 1998) and has some odd behavior. For example, it checks membership in the object or object prototype, so
"hasOwnProperty" in {} // true
"map" in [] // true
"filter" in [] // true
"set" in new URLSearchParams() // trueThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woops python brain kicked in. I switched it to Object.keys().includes
|
@abeglova the current failing test is a flaky one that Matt has fixed in a separate PR |
abeglova
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/9103
Description (What does it do?)
This PR makes it so we can configure what offerers show up as a facet on the search page. In our case we want to remove "bootcamps" and "mit climate" as options.
How can this be tested?
python manage.py update_index --allto sync the flag to opensearchAdditional context
This pr also includes a fix for a flaky test i noticed that is also affecting main